Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Delete user if email isn't confirmable #366

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

mftaff
Copy link

@mftaff mftaff commented Jan 22, 2018

fixes #130

Note: I am unable to test this solution because of the inability to setup locally using vagrant (see #353). However, it is a starting point, and I am happy to make any changes required :-)

@sethherr
Copy link
Member

Hey @mftaff - bummer the vagrant setup isn't working. We definitely need to have tests for this though. Once you push up a change to this pull request, it will run on Travis - so even if you can't run the tests locally, you can add a new test to the spec file and it will be tested on our CI - warning: this isn't the fastest way to do development, it's much better to get things running locally

The place you need to add a spec is in spec/workers/email_confirmation_worker_spec.rb.

The spec can stub raising the exception, it should test that when the exception is raised it deletes the user.

@mftaff
Copy link
Author

mftaff commented Jan 23, 2018

👍 Cool cool. I just got rspec running locally. I am writing a test now.

@mftaff mftaff force-pushed the handle-invalid-emails branch from 8c957f3 to 2c99f16 Compare January 23, 2018 09:09
@mftaff
Copy link
Author

mftaff commented Jan 23, 2018

I am having a bit of trouble with creating the test. It seems that when in test mode, sidekiq isn't actually sending emails: [rspec-sidekiq] WARNING! Sidekiq will *NOT* process jobs in this environment. See https://github.com/philostler/rspec-sidekiq/wiki/FAQ-&-Troubleshooting

I may have misunderstood what that link said, but it seems that when testing, the mailers are just being created, but not actually sent out.

Do you know how to get around this? Some way to actually try and deliver (or test the emails validity) the email and this way we should be able to catch any SMTPSyntaxErrors.

Here is what I have so far:

describe EmailConfirmationWorker do
  ...
  it 'deletes user if email is invalid' do
    user = FactoryGirl.create(:user)
    user.update(email: '[email protected]')
    EmailConfirmationWorker.new.perform(user.id)
    # assuming the EmailConfirmationWorker raised an SMTPSyntaxError the user should be deleted:
    expect(User.count).to be_zero 
  end
end

@mftaff mftaff force-pushed the handle-invalid-emails branch from 2c99f16 to 3c8a705 Compare January 23, 2018 10:39
@mftaff
Copy link
Author

mftaff commented Jan 23, 2018

As an aside I just came up with expect { user.reload }.to raise_error ActiveRecord::RecordNotFound as the way to check if the user was deleted, as apposed to checking User.count which is less specific.

@sethherr
Copy link
Member

Sorry for the slow response @mftaff ! I think checking that the error is raised is good. I think you could actually stub the delivery of the message and the raising of the error, since you're correct, the mailers don't actually deliver in testing.

@mftaff
Copy link
Author

mftaff commented Feb 4, 2018

No worries!

I have been trying to do exactly that - however, so far I haven't been successful. I haven't written tests for mailers before, so this is a bit new for me. Do you have an idea what this test needs to look like?

@sethherr sethherr changed the base branch from master to main June 22, 2020 19:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Handle incorrect emails
2 participants